Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove loadConfigStrict #8915

Closed
wants to merge 2 commits into from
Closed

Remove loadConfigStrict #8915

wants to merge 2 commits into from

Conversation

StefanBratanov
Copy link
Contributor

PR Description

loadConfigStrict and ignoreUnknownConfigItems is only used in tests. When removed getTtfbTimeout and getRespTimeout, there were failing tests since some were using loadConfigStrict, but adding to KEYS_TO_IGNORE in SpecConfigReader will cause incompatibility between old versions of VC and new BN. Not sure what is a good approach in this situation. We can keep the old getters unused in NetworkingSpecConfig, but prefer to have them removed for clarity, but without causing incompatibilities.

Fixed Issue(s)

N/A

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@rolfyone
Copy link
Contributor

why are we removing this? maybe at least move it to testing rather than remove, its kinda important w'ere trying to validate our local configuration...

@StefanBratanov
Copy link
Contributor Author

why are we removing this? maybe at least move it to testing rather than remove, its kinda important w'ere trying to validate our local configuration...

I was thinking how to remove getTtfbTimeout and getRespTimeout from NetworkingSpecConfig without tests failing or without causing compatibility issues (if we put in deprecated fields, the spec endpoint won't return them)

@tbenr
Copy link
Contributor

tbenr commented Dec 17, 2024

I think it is valuable in general to use it in tests because there we want to be less permissive and early detect potential issues. That's a general statement but I don't know if in practice we will use it after you changes.

@StefanBratanov
Copy link
Contributor Author

I think it is valuable in general to use it in tests because there we want to be less permissive and early detect potential issues. That's a general statement but I don't know if in practice we will use it after you changes.

Yeah, I have rethought and it makes sense to keep it, but the question remains how to deal with deprecated fields and not failing tests.

@StefanBratanov
Copy link
Contributor Author

Will close and remove deprecated fields later

@StefanBratanov StefanBratanov deleted the remove_load_config_strict branch December 17, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@StefanBratanov @rolfyone @tbenr